Use fancy new keviyah module for Hebrew calendrical calculations #4504

Manishearth · 2024-01-03T00:24:42Z

progress on #3933

…contains an R.D, and the year length

Manishearth · 2024-01-03T00:30:37Z

Bench results

Before

     Running benches/convert.rs (/home/manishearth/dev/icu4x/target/release/deps/convert-9276c61bbc500b27)
convert/calendar/hebrew time:   [1.3679 µs 1.3884 µs 1.4147 µs]
                        change: [-7.0007% -5.6385% -4.1986%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 5 outliers among 100 measurements (5.00%)
  3 (3.00%) high mild
  2 (2.00%) high severe

     Running benches/date.rs (/home/manishearth/dev/icu4x/target/release/deps/date-b327089746c19b13)
date/calendar/hebrew    time:   [16.184 µs 16.242 µs 16.319 µs]
Found 3 outliers among 100 measurements (3.00%)
  1 (1.00%) high mild
  2 (2.00%) high severe

test result: ok. 0 passed; 0 failed; 3 ignored; 0 measured; 98 filtered out; finished in 0.00s

After:

     Running benches/convert.rs (/home/manishearth/dev/icu4x/target/release/deps/convert-9276c61bbc500b27)
convert/calendar/hebrew time:   [65.360 ns 66.764 ns 68.279 ns]
                        change: [-95.378% -95.290% -95.200%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 3 outliers among 100 measurements (3.00%)
  3 (3.00%) high mild

     Running benches/date.rs (/home/manishearth/dev/icu4x/target/release/deps/date-b327089746c19b13)
date/calendar/hebrew    time:   [2.0123 µs 2.0304 µs 2.0490 µs]
                        change: [-87.846% -87.746% -87.657%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 3 outliers among 100 measurements (3.00%)
  2 (2.00%) high mild
  1 (1.00%) high severe

I think this is sufficient improvement that we do not need to precompute anything. For reference (using bench results from #4468 (comment)), cached chinese has convert timings 83.769 ns and date timings 5.7914 ms , and Ethiopic has convert timings 22.850 ns and date timings 1.2878 µs.

So this is faster than cached chinese, but slower (same order of magnitude) than a simpler calendar.

@sffc if you agree that we do not need to precompute anything I can add an unconditional ::new() method and deprecate ::new_always_calculating()

Manishearth · 2024-01-03T01:00:26Z

Shane is OOO for a couple weeks, if @robertbastian wants to review this instead.

sffc

Didn't get to hebrew_keviyah.rs yet but here are a few comments

sffc · 2024-01-03T22:08:59Z

components/calendar/src/calendar_arithmetic.rs

        if day > max_day {
            return Err(CalendarError::Overflow {
                field: "day",
                max: max_day as usize,
            });
        }

-        Ok(Self::new_unchecked(year, month, day))
+        Ok(Self::new_unchecked_with_info(year, month, day, info))


Nit: Can you debug-assert that info is the correct info for year? That's an invariant of CalendarArithmetic that isn't being enforced in this constructor as far as I see.

Will do. In the long term I want CalendarArithmetic's methods to always take ArithmeticDate (even though it means in some cases we'll need to make fake dates)

Yeah, this isn't possible to do; CalendarArithmetic doesn't know anything about YearInfo.

sffc · 2024-01-03T22:13:50Z

components/calendar/src/hebrew.rs

@@ -531,86 +557,35 @@ mod tests {
            (1, TEVET, 5783),
        ];

-        let civil_hebrew_dates: [(u8, u8, i32); 48] = [


Issue: why are you removing the civil_hebrew_dates assertions?

the code being tested doesn't exist anymore. It was needless complexity and the book/civil thing isn't a huge issue

Also the civil data had bugs.

I don't understand. This table of civil Hebrew dates corresponds to the ISO dates above. We can and should test that the keviyah code produces the correct results.

The book table is easier to read: it uses the month constants (so it's easier to verify).

We are testing that the keviyah code produces the correct results.

Previously the Hebrew calendar impl did a lot of civil/book conversions and the two tables were testing that. Now that's just two tables representing the same information.

If we keep either table though I'm saying we should keep the civil table because that's the public behavior. Book Hebrew is only for internal things and isn't exposed through API. It's easier to read because we can use constants for the months since the leap month is at the end. I wrote the ordinal constants with Pedro.

(re: FormattableMonth yeah, I was talking about construction only)

I was hoping to keep them in earlier and added some asserts about them that seemed to fail. Those asserts tested code I was deleting anyway so I deleted them. I didn't spend effort verifying but I did verify the const based data was correct.

I do not think that table is useful from a maintenance or review standpoint. I am willing to add a test for ordinal construction that manually does the leap year month offsetting for the known leap years in the data (I think there's only one) and then uses the same data. I don't want us to have two tables, that's more inscrutable bags of test data that we don't need.

Would you be open to merging the three arrays into a single array of tuples?

I'm planning on merging the two arrays. I'd rather not re-add the manual ordinal one. I'll do the thing I proposed where I test ordinal code via checking for known leap years.

I was happy to have the ordinal constants back when we had the whole civil/book thing and plenty of ways that conversion could go wrong. Now that that's completely gone, I would rather have this one table.

I've added tests for ordinal stuff and cleaned up the tables.

components/calendar/src/hebrew.rs

utils/calendrical_calculations/src/hebrew_keviyah.rs

sffc · 2024-01-04T18:02:42Z

components/calendar/src/hebrew.rs

@@ -531,86 +557,35 @@ mod tests {
            (1, TEVET, 5783),
        ];

-        let civil_hebrew_dates: [(u8, u8, i32); 48] = [


I don't understand. This table of civil Hebrew dates corresponds to the ISO dates above. We can and should test that the keviyah code produces the correct results.

Co-authored-by: Shane F. Carr <shane@unicode.org>

sffc

Thanks @Manishearth

sffc · 2024-01-05T05:01:17Z

components/calendar/src/types.rs

+    /// Construct a "normal" month code given a number ("Mxx").
+    ///
+    /// Returns an error for months greater than 99
+    pub fn new_normal(number: u8) -> Option<Self> {


Thought: This is new public API. We should probably bikeshed. But it seems okay.

yeah I wasn't sure about it either but I've reached for this kind of thing before.

I made it cfg(test) for now. I didn't feel strongly about it. We can discuss separately.

Manishearth added 6 commits January 2, 2024 16:12

Improve Date's debug impl

94e12ea

Add conversiomn bench for hebrew

93cb820

Functions for getting YearInfo from Keviyah, computing the year that …

ae80409

…contains an R.D, and the year length

Add month length code

8d3931b

inline

127d50e

Plug keviyah into hebrew calendar impl

b805dde

Manishearth requested a review from sffc as a code owner January 3, 2024 00:24

Manishearth requested a review from sffc January 3, 2024 00:24

Manishearth requested a review from a team as a code owner January 3, 2024 00:24

Manishearth mentioned this pull request Jan 3, 2024

Loading cache data for lunar calendars #3933

Closed

14 tasks

fixes

55c8256

Manishearth force-pushed the keviyah-use branch from 7c76df5 to 55c8256 Compare January 3, 2024 00:47

sffc reviewed Jan 3, 2024

View reviewed changes

Manishearth added 3 commits January 3, 2024 18:49

Add month_day_for(), cleanup

8050924

clippy

3b8a6c4

test

fdbb0cb

sffc reviewed Jan 4, 2024

View reviewed changes

Manishearth and others added 9 commits January 4, 2024 18:04

Update utils/calendrical_calculations/src/hebrew_keviyah.rs

1a9ed56

Co-authored-by: Shane F. Carr <shane@unicode.org>

Better length correction APIs

201b4b9

saturate

1bc5496

Clamp to min/max r.d.

bebd22a

Write days_preceding as match statement

88f11b1

clip

9e01978

Add normalized month constants

162011e

Remove all book code

2782bae

Use consistent spelling of month names

d8013c1

Manishearth requested a review from sffc January 4, 2024 22:29

merge table

c3aefa5

Manishearth added 2 commits January 4, 2024 18:37

test ordinal construction

0325fb7

make table uniform

6b63138

sffc previously approved these changes Jan 5, 2024

View reviewed changes

clippy

81c1e70

Manishearth dismissed sffc’s stale review via 81c1e70 January 5, 2024 05:35

pubcrate

fb1ce60

Manishearth requested a review from sffc January 5, 2024 05:37

sffc approved these changes Jan 5, 2024

View reviewed changes

Manishearth merged commit 1ae5bb3 into unicode-org:main Jan 5, 2024
29 checks passed

Manishearth deleted the keviyah-use branch January 5, 2024 23:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use fancy new keviyah module for Hebrew calendrical calculations #4504

Use fancy new keviyah module for Hebrew calendrical calculations #4504

Manishearth commented Jan 3, 2024 •

edited

Loading

Manishearth commented Jan 3, 2024 •

edited

Loading

Manishearth commented Jan 3, 2024

sffc left a comment

sffc Jan 3, 2024

Manishearth Jan 3, 2024

Manishearth Jan 4, 2024

sffc Jan 3, 2024

Manishearth Jan 3, 2024

sffc Jan 4, 2024

Manishearth Jan 4, 2024 •

edited

Loading

sffc Jan 4, 2024

Manishearth Jan 5, 2024 •

edited

Loading

sffc Jan 5, 2024

Manishearth Jan 5, 2024 •

edited

Loading

Manishearth Jan 5, 2024

Manishearth Jan 5, 2024

sffc Jan 4, 2024

sffc left a comment

sffc Jan 5, 2024

Manishearth Jan 5, 2024

Use fancy new keviyah module for Hebrew calendrical calculations #4504

Use fancy new keviyah module for Hebrew calendrical calculations #4504

Conversation

Manishearth commented Jan 3, 2024 • edited Loading

Manishearth commented Jan 3, 2024 • edited Loading

Manishearth commented Jan 3, 2024

sffc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Manishearth Jan 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Manishearth Jan 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Manishearth Jan 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sffc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Manishearth commented Jan 3, 2024 •

edited

Loading

Manishearth commented Jan 3, 2024 •

edited

Loading

Manishearth Jan 4, 2024 •

edited

Loading

Manishearth Jan 5, 2024 •

edited

Loading

Manishearth Jan 5, 2024 •

edited

Loading